Add human-readable descriptions to CheckCode returns in modules#21359
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request standardizes Metasploit module check results by adding human-readable description strings to Exploit::CheckCode/CheckCode returns so that status/details bubble up to users more consistently.
Changes:
- Adds descriptive messages to
Safe/Detected/Appears/Vulnerable/Unknowncheck results across many exploit modules. - Makes several checks report version-based context in the returned
CheckCodemessage. - Aligns modules with the ongoing effort from #21304 to improve check output quality.
Reviewed changes
Copilot reviewed 112 out of 112 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/exploits/unix/webapp/zpanel_username_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zoneminder_snapshots.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zoneminder_packagecontrol_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zimbra_lfi.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zeroshell_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/xymon_useradm_cmd_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/xoda_file_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_total_cache_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/wp_property_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_plainview_activity_monitor_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_pixabay_images_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_phpmailer_host_header.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/wp_optimizepress_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_infusionsoft_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_google_document_embedder_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_foxypress_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_asset_manager_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_advanced_custom_fields_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_admin_shell_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/webtester_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/webmin_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/webmin_show_cgi_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/vicidial_user_authorization_unauth_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/vicidial_manager_send_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/vicidial_agent_authenticated_rce.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/vbulletin_vote_sqli_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/twiki_search.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/twiki_maketext.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/twiki_history.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tuleap_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tuleap_rest_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/trixbox_langchoice.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/trixbox_ce_endpoint_devicemap_rce.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/tikiwiki_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tikiwiki_jhot_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tikiwiki_graph_formula_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/squash_yaml_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/sphpblog_file_upload.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/skybluecanvas_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/sixapart_movabletype_storable_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/simple_e_document_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/seportal_sqli_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/rconfig_install_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/projectsend_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/projectpier_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/phpmyadmin_config.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/phpcollab_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/php_xmlrpc_eval.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_vbulletin_template.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_include.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_eval.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_charts_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/oracle_vm_agent_utl.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/openx_banner_edit.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/opensis_modname_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/opensis_chain_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/opennetadmin_ping_cmd_injection.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/openmediavault_rpc_rce.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/openemr_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/openemr_sqli_privesc_upload.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/open_flash_chart_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/nextcloud_workflows_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/narcissus_backend_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/nagios_graph_explorer.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/nagios3_history_cgi.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/mybb_backdoor.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/moinmoin_twikidraw.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/maarch_letterbox_file_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/libretto_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/kimai_sqli.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/jquery_file_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_tinybrowser.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_media_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_contenthistory_sqli_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_comjce_imgmanager.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/joomla_comfields_sqli_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_akeeba_unserialize.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/invision_pboard_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/instantcms_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/hybridauth_install_php_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/horde_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/havalite_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/hastymail_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/graphite_pickle_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/google_proxystylesheet_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/get_simple_cms_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/fusionpbx_operator_panel_exec_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/fusionpbx_exec_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/freepbx_config_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/foswiki_maketext.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/flashchat_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/elfinder_php_connector_exiftran_cmd_injection.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/egallery_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/drupal_restws_unserialize.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/drupal_restws_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/drupal_drupalgeddon2.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/drupal_coder_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/dogfood_spell_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/datalife_preview_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/coppermine_piceditor.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/clipbucket_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/citrix_access_gateway_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/carberp_backdoor_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/byob_unauth_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/basilic_diff_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/barracuda_img_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/awstatstotals_multisort.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/awstats_migrate_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/awstats_configdir_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/arkeia_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/ajenti_auth_username_cmd_injection.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/actualanalyzer_ant_cookie_exec.rb | Adds descriptive strings to check return codes. |
| flow_id = create_workflow('sleep 1') | ||
|
|
||
| Exploit::CheckCode::Safe('Target is not vulnerable') if flow_id.nil? | ||
|
|
||
| delete_workflow(flow_id) | ||
| Exploit::CheckCode::Vulnerable | ||
| Exploit::CheckCode::Vulnerable('The target is vulnerable') | ||
| end |
There was a problem hiding this comment.
Exploit::CheckCode::Safe(...) if flow_id.nil? doesn't return, so the method continues, calls delete_workflow(nil), and then returns Vulnerable even when flow_id wasn't created. This should be return Exploit::CheckCode::Safe(...) if flow_id.nil? (or an Unknown/Detected result if workflow creation failed for another reason).
| ) | ||
|
|
||
| if res and res.code == 500 or res.body =~ /PHP_Incomplete_Class/ | ||
| return Exploit::CheckCode::Vulnerable | ||
| return Exploit::CheckCode::Vulnerable('The target is vulnerable') | ||
| elsif res and res.code == 200 | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
The condition if res and res.code == 500 or res.body =~ /PHP_Incomplete_Class/ can evaluate res.body even when res is nil (due to and/or precedence), which would raise a NoMethodError during check. Consider rewriting as if res && (res.code == 500 || res.body =~ /PHP_Incomplete_Class/) (and guarding res.body).
| def check | ||
| response, version = find_setup_path(mode: :check) | ||
| return Exploit::CheckCode::Safe unless response&.code == 200 | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") unless response&.code == 200 | ||
|
|
There was a problem hiding this comment.
If find_setup_path can't find a setup page (or can't extract a version), version may be nil but this returns Safe("Version #{version} is not vulnerable"), producing a confusing message. Consider returning a message that reflects the real condition (e.g., "phpMyAdmin setup page not found" / "Could not determine version") and avoid interpolating version unless present.
| @@ -76,12 +76,12 @@ def check | |||
| version = res.body.scan(/>HybridAuth (2\.[012]\.[\d\.]+(-dev)?) Installer</).first.first | |||
| vprint_status "Found version: #{version}" | |||
| if version =~ /^2\.(0\.(9|10|11)|1\.[\d]+|2\.[012])/ | |||
| return Exploit::CheckCode::Vulnerable | |||
| return Exploit::CheckCode::Vulnerable("Detected vulnerable version #{version}") | |||
| else | |||
| vprint_error "HybridAuth version #{version} is not vulnerable" | |||
| end | |||
| end | |||
| Exploit::CheckCode::Safe | |||
| Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | |||
| end | |||
There was a problem hiding this comment.
version is only set when the installer page matches the version regex, but the method always falls through to Safe("Version #{version} is not vulnerable"). In most non-vulnerable cases version will be nil here (404 / not writable / unrecognized response), resulting in "Version is not vulnerable". Consider returning more specific Safe/Detected/Unknown results for each branch and only include the version in the message when it's actually extracted.
| res = send_request(rev_url + Rex::Text.uri_encode(rev)) | ||
| if (not res) or (res.code != 200) | ||
| vprint_warning("Error with exploit request (HTTP #{res.code}, should be 200)") unless res.code == 200 | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
In the (not res) || (res.code != 200) branch, res can be nil but the warning still calls res.code, which will raise a NoMethodError during check. Consider handling the nil-response case separately before referencing res.code.
| elsif res.body =~ /Kimai/ and res.body =~ /(0\.9\.[\d\.]+)<\/strong>/ | ||
| version = "#{$1}" | ||
| print_good("Found version: #{version}") | ||
| if version >= "0.9.2" and version <= "0.9.2.1306" | ||
| return Exploit::CheckCode::Appears | ||
| return Exploit::CheckCode::Appears("Version #{version} appears to be vulnerable") | ||
| end | ||
| end | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
version is only set when the response matches the Kimai version regex; otherwise it's nil but the method still returns Safe("Version #{version} is not vulnerable"), resulting in an empty version in the output. Consider returning a generic Safe('The target is not vulnerable') when the version can't be determined, or explicitly set a fallback value/message.
| if (res and res.body =~ /Simple PHP Blog (\d)\.(\d)\.(\d)/) | ||
|
|
||
| ver = [ $1.to_i, $2.to_i, $3.to_i ] | ||
| vprint_status("Simple PHP Blog #{ver.join('.')}") | ||
|
|
||
| if (ver[0] == 0 and ver[1] < 5) | ||
| if (ver[1] == 4 and ver[2] > 0) | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{ver} is not vulnerable") | ||
| end | ||
|
|
||
| return Exploit::CheckCode::Appears | ||
| return Exploit::CheckCode::Appears("Version #{ver} appears to be vulnerable") | ||
| end | ||
| end | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{ver} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
ver is an array (e.g., [0, 4, 0]) and may be nil when the version regex doesn't match, but it's interpolated directly in both Safe and Appears messages. This produces outputs like "Version [0, 4, 0] ..." or "Version is not vulnerable". Consider using ver.join('.') for messages and falling back to a generic Safe message when the version can't be parsed.
| version = get_target(res) | ||
| if version.nil? | ||
| # We don't print out an error message here as returning this will | ||
| # automatically cause Metasploit to print out an appropriate error message. | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
When get_target returns nil, this returns Safe("Version #{version} is not vulnerable") where version is nil, resulting in a misleading message. Consider returning a generic Safe('The target is not vulnerable') (or Detected with a "version unsupported"/"unknown" message) in this branch.
| if (not res) or (res.code != 200) | ||
| vprint_warning("Error with exploit request (HTTP #{res.code}, should be 200)") unless res.code == 200 | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
In the (not res) || (res.code != 200) branch, res can be nil but the warning still calls res.code, which will raise a NoMethodError during check. Consider guarding the log message (only reference res.code when res is present) or restructuring to handle the nil-response case separately.
| if res.body and (res.body =~ /Performance optimized by W3 Total Cache/ or res.body =~ /Cached page generated by WP-Super-Cache/) | ||
| return Exploit::CheckCode::Detected | ||
| return Exploit::CheckCode::Detected("Detected version #{version}") | ||
| end | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
version is only assigned inside the X-Powered-By header branch, but it's referenced in the later Detected/Safe returns. When the header isn't present, this returns messages like "Detected version " / "Version is not vulnerable". Consider avoiding version in those messages unless it was actually extracted, or set a separate message when W3 Total Cache is only detected via body markers.
adfoster-r7
force-pushed
the
improve-checkcode-messages-12
branch
from
0a7f161 to
f23d3bf
Compare
adfoster-r7
force-pushed
the
improve-checkcode-messages-12
branch
2 times, most recently
from
082ef65 to
50ce997
Compare
| rescue ::Rex::ConnectionRefused, ::Rex::HostUnreachable, ::Rex::ConnectionTimeoutp | ||
| vprint_error("Connection failed") | ||
| return Exploit::CheckCode::Unknown | ||
| return Exploit::CheckCode::Unknown('Could not determine the target status') |
There was a problem hiding this comment.
Critical: Problem: rescue references ::Rex::ConnectionTimeoutp, which does not exist and will prevent timeout errors from being caught. Impact: check may raise unexpectedly instead of returning Unknown, breaking module behavior on network timeouts. Fix: change it to ::Rex::ConnectionTimeout (matching the rest of the module).
| end | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") |
There was a problem hiding this comment.
Important: Problem: when version is nil, return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") produces an unhelpful message like "Version is not vulnerable". Impact: users get confusing check output when the setup page is found but the version cannot be determined. Fix: only include the version in the message when it is present, otherwise return a generic Safe/Detected message that does not interpolate nil.
adfoster-r7
force-pushed
the
improve-checkcode-messages-12
branch
from
50ce997 to
0bf595c
Compare
| res = login(user, pass) | ||
| unless res | ||
| print_error("No response was received from #{peer} whilst in check(), check it is online and the target port is open!") | ||
| return CheckCode::Detected | ||
| return CheckCode::Detected('The target service was detected') | ||
| end |
There was a problem hiding this comment.
Important: Problem: when login returns no response, check prints an error but returns CheckCode::Detected('The target service was detected') (line 147), which contradicts the condition and misreports status. Impact: users may see a false-positive detection even though connectivity is unknown. Fix: return CheckCode::Unknown (or similar) with a message reflecting the connection failure instead of Detected.
| res = execute_command(cmd) | ||
| t2 = Time.now.to_i | ||
| unless res | ||
| print_error("#{peer} - Connection failed whilst trying to perform the code injection.") | ||
| return CheckCode::Detected | ||
| return CheckCode::Detected("Detected version #{version}") | ||
| end |
There was a problem hiding this comment.
Important: Problem: on a nil response from execute_command (connection failed during the injection verification), check returns CheckCode::Detected("Detected version #{version}") even though vulnerability could not be verified. Impact: callers may treat this as a positive detection rather than an indeterminate check due to connectivity issues. Fix: return CheckCode::Unknown(...) (optionally including the detected version in the message) when the verification request fails.
Release NotesImproves multiple module check code messages and statuses. |
Improves multiple module check code messages and statuses
This metadata is currently missing in modules, which means the bubbling up of results to users is often missing
Continuation of #21304
Verification